-
Notifications
You must be signed in to change notification settings - Fork 3.9k
fix: handle JSON contents of create_new_file #8973
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Handle case where JSON content was parsed into an object by the tool call parser. If the arguments to the tool call are valid JSON (e.g. the model attempts to create a .json file) the earlier call to JSON.parse() will have deeply parsed the returned contents. If that has happened, convert back to string. Fixes: continuedev#8972
|
Keep this PR in a mergeable state → Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
|
Keep this PR in a mergeable state → Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
1 similar comment
|
Keep this PR in a mergeable state → Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcowger could we add this special handling in createFileImpl instead of here?
OR, I think you could just remove special handling for contents and always make sure it's a string within getStringArg!
|
Reviewed the PR changes. This is an internal bug fix in the tool argument parser that handles JSON content correctly when creating JSON files. The changes don't require documentation updates since:
The fix ensures that when models create .json files with JSON content, the parser correctly handles the case where the content gets parsed into an object and converts it back to a string. |
|
I have read the CLA Document and I hereby sign the CLA |
Good call. Removed the special case handling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 2 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 2 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcowger this looks good to me. I haven't been able to replicate the failing case locally - it seems like claude and openai models mostly escape JSON strings correctly. Could you share which model you find this error often happens with?
EDIT I see M2 in the issue but wasn't sure which model that was. Model + provider combo would be helpful
|
I can replicate on Synthetic.new. It doesn't happen for me on OpenRouter either. Happy to provide a key privately for you to test so you dont have to give them money. |
|
@mcowger which model are you working with? |
Minimax M2 (note, I cannot replicate this on OpenRouter). I cannot replicate this via GLM4.6 on the same provider, so it appears to be an interaction between the inference and the model and continue. |
|
I captured traces of the differences in the output: https://gist.github.com/mcowger/3aea3a2314d9f5540e7bbf68d9cafcd7 GLM outputs double escaped strings, while M2 does not. That should be enough to replicate with? |
|
This seems like could be a valid issue for other models as well, and if the tool implementation is looking for a string arg specifically, like the create new file tool, this fix seems fine. I guess you would still see issues with MCP tools when they expect string JSON and receive JSON object. The solution might be to detect |
|
Got it, thanks for the traces. It's no clear why the model would escape differently between providers (as far as I understand this would only happen because of something on the provider side) but I could see this happening in other situations as well and this fix won't hurt. |
|
Agree its a provider side issue, and I've filed a bug with them. I dont use MCPs much, but I agree it could impact them as well. |
RomneyDa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good for now, can address MCP on a separate issue if it comes up
|
🎉 This PR is included in version 1.35.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Handle case where JSON content was parsed into an object by the tool call parser. If the arguments to the tool call are valid JSON (e.g. the model attempts to create a .json file) the earlier call to JSON.parse() will have deeply parsed the returned contents. If that has happened, convert back to string.
Fixes: #8972
Description
[ What changed? Feel free to be brief. ]
AI Code Review
@continue-reviewChecklist
Screen recording or screenshot
NA
Tests
Added 6 tests to
core/tools/parseArgs.vitest.tsfor new check.Summary by cubic
Fixes failures when create_new_file receives JSON content parsed into an object by ensuring contents is always a string. Prevents crashes when creating .json files. Fixes #8972.
Written for commit e04c21f. Summary will update automatically on new commits.